Use builder to init output stream #641
Conversation
Incompatible API change.
THe idea was to add a new function.
Add an example for rodio::play() Remove experimental API from rustdocs.
| pub sample_rate: SampleRate, | ||
| pub buffer_size: BufferSize, | ||
| pub sample_format: SampleFormat, | ||
| } |
There was a problem hiding this comment.
This can be a part of the builder, but I wanted to give users an option co copy/store/modify it if they prefer, so the builder becomes only an optional convenience.
There was a problem hiding this comment.
I would not make it pub. If we make it pub we can never change the fields or add one without it being a breaking change. By using the builder we are free to add more in the future.
There was a problem hiding this comment.
The motivation of making this a separate struct, was to let users to pass it around or keep it in configs somewhere, for example. Yes, there is no problem to not publish it just yet.
There was a problem hiding this comment.
If I understand correctly OutputStreamConfig is public to allow users to skip using the builder a second time.
One of the big advantages to builders is that we can expand them over time without that being a breaking change.
My advice is to remove OutputStreamBuilder completely from the public API. Users wanting to abstract away the builder can still do that in their own project.
There was a problem hiding this comment.
Usually I prefer plain data if possible. But in this case I have allowed this option (to use the config directly) just to cover all the cases. But since no one actually requested this, I do not object hiding the struct.
Updated.
There was a problem hiding this comment.
OutputStreamBuilder fields no longer need to be public (it compiles with pub removed).
Sorry I would apply the change to the PR myself but its such a chore to set that up. Ill see if I can automate that better on my side so its no longer a problem in the future.
There was a problem hiding this comment.
you know what ill fix it post merge, cause this is all ready to go! 🥳
|
It looks like those "missing documentation" errors do not show up when building with stable toolchain : Using beta or nightly does show them. |
|
I have not done a full review (given its still a draft) but looked through it quickly and tried to address all the comments you made. What I see looks great though 🥳 ! It is clear you carefully considered your choices. Thank you very much for all the work. Ill be careful not to create any merge conflicts. Given how big this PR is ill stay away from making larger changes till this has landed. |
|
@dvdsk I have added dome docs, so in this form I'd say it is complete. The biggest reason I titled it as a |
PetrGlad
changed the title
|
Only thing I still have a problem with is the We do still need a changelog entry (see CHANGELOG.md) and ideally we should add a porting_guide.md given this is a breaking change. Could you make those or shall I get on that post merge? |
|
I have added a custom config example (doubles as a test for the builder). |
|
@dvdsk Updated, please have a look. |
|
This now also resolves: #643 |
|
Thanks a lot for all the hard work Petr. This has really improved the API 👍 |
This is a breaking change.
Resolves PR#512 and #613
This pull request may be related #547
Still missing: